Skip to content

[Storage] Refactor index execution result #8005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 78 commits into
base: master
Choose a base branch
from
Open

[Storage] Refactor index execution result #8005

wants to merge 78 commits into from

Conversation

zhangchiqing
Copy link
Member

@zhangchiqing zhangchiqing commented Oct 3, 2025 •
edited
Loading

Work towards #7912

  • Added Key Existence checks to the db operations when saving execution results.
  • Renamed the lock IDs, see comments for motivation.
  • Combined some storage API, such as BatchInsert and BatchIndex and combined into BatchInsertAndIndexXXX

@zhangchiqing zhangchiqing changed the title [Storage] Refactor index result [Storage] Refactor index execution result Oct 3, 2025
@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 43.58974% with 220 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
storage/operation/events.go 23.25% 29 Missing and 4 partials ⚠️
storage/mock/execution_results_reader.go 0.00% 22 Missing ⚠️
storage/mock/execution_results.go 0.00% 20 Missing ⚠️
storage/operation/results.go 9.09% 20 Missing ⚠️
storage/mock/blocks.go 0.00% 12 Missing ⚠️
storage/operation/headers.go 40.00% 8 Missing and 4 partials ⚠️
storage/operation/receipts.go 35.29% 8 Missing and 3 partials ⚠️
storage/store/events.go 38.46% 7 Missing and 1 partial ⚠️
...ine/access/ingestion2/finalized_block_processor.go 61.11% 5 Missing and 2 partials ⚠️
engine/access/ingestion/engine.go 68.42% 4 Missing and 2 partials ⚠️
... and 19 more

📢 Thoughts on this report? Let us know!

zhangchiqing
zhangchiqing commented Oct 8, 2025
View reviewed changes
cmd/util/cmd/root.go
rootCmd.AddCommand(leaders.Cmd)
rootCmd.AddCommand(epochs.RootCmd)
rootCmd.AddCommand(edbs.RootCmd)
rootCmd.AddCommand(index_er.RootCmd)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tool is no longer useful since this PR. Because now we don't allow multiple results to be stored for the same block, therefore we don't have multiple results to choose from which one to be reindexed with.

zhangchiqing
zhangchiqing commented Oct 8, 2025
View reviewed changes
engine/access/ingestion2/collection_syncer.go
ctx.Throw(fmt.Errorf("could not acquire lock for collection indexing: %w", err))
return
}
defer lctx.Release()
Copy link
Member Author

@zhangchiqing zhangchiqing Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mistake like this will happen, in order to avoid this, I will create a separate PR to replace all AcquireLock usages with storage.WithLock

@zhangchiqing zhangchiqing marked this pull request as ready for review October 9, 2025 17:04
@zhangchiqing zhangchiqing requested a review from a team as a code owner October 9, 2025 17:04
@zhangchiqing zhangchiqing mentioned this pull request Oct 10, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jordanschalm jordanschalm Awaiting requested review from jordanschalm

@AlexHentschel AlexHentschel Awaiting requested review from AlexHentschel

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@zhangchiqing @codecov-commenter